Skip to content

Conversation

@EvilOlaf
Copy link
Member

@EvilOlaf EvilOlaf commented Oct 22, 2025

Description

  • bump uboot to latest stable
  • adjust aic8800 extension to build against 6.18

Note: Tried to get rid of vendor uboot altogether but board naming differs, therefore uboot fails to find fdt file.

How Has This Been Tested?

  • build/boot edge or 3E
  • test wifi on 3W- nope, no hw

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules

@github-actions github-actions bot added 11 Milestone: Fourth quarter release size/small PR with less then 50 lines Needs review Seeking for review Hardware Hardware related like kernel, U-Boot, ... Framework Framework components labels Oct 22, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 22, 2025

Walkthrough

The pull request updates Radxa Zero3 board configuration by renaming and rewriting the mainline U-Boot handler function from post_family_config__use_mainline_uboot_except_vendor() to post_family_config__use_mainline_uboot(), inverting its logic to enable mainline U-Boot for all non-vendor branches instead of restricting it to current/edge branches. The BOOTBRANCH default is updated from tag:v2025.10-rc2 to tag:v2025.10. Additionally, the AIC8800 kernel DKMS installation threshold in extensions/radxa-aic8800.sh is raised from kernel 6.18 to 6.19, allowing kernel 6.18.x systems to proceed with DKMS installation instead of skipping.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

The changes involve two different files with relatively straightforward logic modifications. While heterogeneous in nature (branch control flow inversion vs. kernel version threshold adjustment), each change is simple and focused without extensive structural reorganization or dense logic patterns.

Possibly related PRs

Suggested labels

Needs review, size/medium, Hardware, Patches

Suggested reviewers

  • igorpecovnik
  • pyavitz
  • RadxaYuntian
  • amazingfate
  • chainsx

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check ❓ Inconclusive The title "radxa-zero3 maintenance" is vague and generic, using a non-descriptive term that doesn't convey meaningful information about the specific changes. While it correctly identifies the board being updated, it fails to summarize the actual changes, which include bumping U-Boot to the latest stable version and adjusting the aic8800 extension for kernel 6.18. A reader scanning the pull request history would not understand the primary purpose of these changes from the title alone. Consider revising the title to be more specific and descriptive of the actual changes, such as "radxa-zero3: bump U-Boot to v2025.10 and adjust aic8800 for kernel 6.18" to clearly communicate the main objectives to future reviewers.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed The pull request description is directly related to the changeset. It accurately describes the two main changes: bumping U-Boot to the latest stable version (reflected in the BOOTBRANCH update from v2025.10-rc2 to v2025.10) and adjusting the aic8800 extension to build against 6.18 (reflected in the kernel version check modification). The author also provides a note about attempting to remove vendor U-Boot entirely, which aligns with the function renaming and logic changes in the configuration file. The description provides meaningful context about what was changed and includes testing details.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
config/boards/radxa-zero3.csc (1)

59-61: LGTM: Cleaner logic and accurate function naming.

The function rename and logic inversion improve code clarity. The new implementation achieves the same result (enabling mainline U-Boot for current/edge branches while keeping vendor on vendor U-Boot) but with simpler, more maintainable code. The inverted condition [[ "${BRANCH}" == "vendor" ]] is easier to understand than the double-negative pattern it replaces.

The new approach is also more general—any future non-vendor branches will automatically use mainline U-Boot without code changes.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 7be3ce0 and d73ac88.

📒 Files selected for processing (2)
  • config/boards/radxa-zero3.csc (2 hunks)
  • extensions/radxa-aic8800.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T06:32:29.806Z
Learnt from: amazingfate
PR: armbian/build#8619
File: config/sources/families/rockchip.conf:222-230
Timestamp: 2025-09-14T06:32:29.806Z
Learning: In the Armbian build system, the write_uboot_platform() function implementations follow different patterns across Rockchip family files. The newer standard (used in rockchip64_common.inc and rk3506) includes 'status=none' parameter in dd commands, while older implementations (rk3288, rk322x) use an older pattern without this parameter. The rk3506 implementation correctly follows the current Rockchip family standard.

Applied to files:

  • config/boards/radxa-zero3.csc
🧬 Code graph analysis (2)
extensions/radxa-aic8800.sh (2)
extensions/v4l2loopback-dkms.sh (1)
  • post_install_kernel_debs__build_v4l2loopback_dkms_kernel_module (15-25)
extensions/bcmdhd.sh (1)
  • post_install_kernel_debs__install_bcmdhd_dkms_package (11-47)
config/boards/radxa-zero3.csc (1)
config/sources/vendors/mekotronics/mekotronics-rk3588.hooks.sh (2)
  • post_family_config__meko_use_mainline_uboot (22-42)
  • post_family_config__vendor_uboot_mekotronics (8-16)
🔇 Additional comments (2)
extensions/radxa-aic8800.sh (1)

13-16: Kernel version threshold adjustment looks correct.

The change from ge 6.18 to ge 6.19 appropriately enables DKMS installation for Linux 6.18 kernels, aligning with the PR objective. The implementation follows the same pattern used in other DKMS extensions.

However, note that Wi-Fi testing on the 3W variant (which uses this AIC8800 extension) was not performed per the PR description. Please verify Wi-Fi functionality on 6.18 kernels when hardware becomes available, or document this limitation if the 3W variant is commonly used.

config/boards/radxa-zero3.csc (1)

66-66: U-Boot version bump to stable is confirmed and ready.

The update from v2025.10-rc2 to v2025.10 is valid—the tag exists in the upstream repository—and the PR reports successful build and boot testing.

@igorpecovnik
Copy link
Member

igorpecovnik commented Oct 22, 2025

https://paste.armbian.com/acidafenox

Connected to XXXXXXXXXXXXX (on wlan0)
	SSID: XXXXXXXXXXXXXXXX
	freq: 5500.0
	RX: 2188 bytes (12 packets)
	TX: 3544 bytes (36 packets)
	signal: -86 dBm
	rx bitrate: 6.0 MBit/s
	tx bitrate: 51.6 MBit/s 40MHz HE-MCS 2 HE-NSS 1 HE-GI 0 HE-DCM 0

@github-actions github-actions bot added the Ready to merge Reviewed, tested and ready for merge label Oct 22, 2025
@github-actions
Copy link
Contributor

✅ This PR has been reviewed and approved — all set for merge!

@github-actions github-actions bot removed the Needs review Seeking for review label Oct 22, 2025
@igorpecovnik igorpecovnik merged commit cc671e2 into armbian:main Oct 22, 2025
12 checks passed
@EvilOlaf EvilOlaf deleted the zero3maint branch October 22, 2025 09:13
@rpardini
Copy link
Member

Note: Tried to get rid of vendor uboot altogether but board naming differs, therefore uboot fails to find fdt file.

@EvilOlaf That could be work-around'ed:

function post_family_config_branch_vendor__zero3_different_dtb_for_vendor() {
	declare -g BOOT_FDT_FILE="rockchip/rk3566-radxa-zero3.dtb"
	display_alert "$BOARD" "Using ${BOOT_FDT_FILE} for ${BRANCH}" "warn"
}

@EvilOlaf
Copy link
Member Author

Interesting, I'll try that tomorrow if I find some time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

11 Milestone: Fourth quarter release Framework Framework components Hardware Hardware related like kernel, U-Boot, ... Ready to merge Reviewed, tested and ready for merge size/small PR with less then 50 lines

Development

Successfully merging this pull request may close these issues.

3 participants